Add minimal support for 4.x #733
Conversation
scotttrinh
force-pushed
the
fix-multirange-anytype
branch
from
58420a4 to
872bcd2
Compare
|
Would this fix the 'Cannot find 'anytype' in multirangestd::anypoint' error while introspecting the database schema, when generating query builder? (EdgeDB 4.x alpha) Edit: Currently running into this issue, but I'm guessing the error lies in where the client doesn't support a subset of changes the server has undergone from 3.x to 4.x |
That's right: it's been failing for some time now with that message, and I was hoping there would be an easy (yet wrong) workaround for this ahead of actually implementing support, but alas, nothing just fell into my lap yesterday. Hope to get multirange support for 4.x-alpha in the next few weeks though, stay tuned! |
|
@scotttrinh Awesome! I'll be keeping an eye out :) Currently depending on it, so I'll just revert to the client.query methods for now! |
|
@bastien8060 You're depending on 4.0-alpha? |
|
@scotttrinh Actually sort of, so my application is rather latency critical, and it seems the first query is much faster on 4.0-alpha than on 3.x, but still not quite there imo. And it's funny, but it's not an issue I noticed on the python client. I might submit an issue on the JS client I think— On the JS client, the first request always takes a couple seconds, regardless of how long the client has been initialized for. const baseClient = createClient();
const configClient = baseClient.withConfig({
"session_idle_transaction_timeout": Duration.from({ seconds: 0 }),
});
const client = await configClient.ensureConnected();Edit: I created #740 |
scotttrinh
force-pushed
the
fix-multirange-anytype
branch
5 times, most recently
from
f3d40a4 to
e4286f9
Compare
scotttrinh
force-pushed
the
fix-multirange-anytype
branch
from
e4286f9 to
54f0ee3
Compare
scotttrinh
force-pushed
the
fix-multirange-anytype
branch
from
0038d2f to
5379b1d
Compare
scotttrinh
changed the title
| @@ -310,14 +310,15 @@ describe("select", () => { | |||
| test("* in polymorphic", async () => { | |||
| const q = e.select(e.Person, () => ({ | |||
| ...e.is(e.Hero, e.Hero["*"]), | |||
| name: true, | |||
There was a problem hiding this comment.
This change reflects the new behavior of 4.x where the parent type's cardinality is considered when using polymorphic queries. In this case, Person has a required name, but the query can return Person records that are not Heros, which would return a empty set name, which is invalid for Person.
|
|
||
| toJSON() { | ||
| return { | ||
| ranges: this._ranges, |
There was a problem hiding this comment.
I think this should probably just return the array, to mirror edgedb's json representation of multiranges.
There was a problem hiding this comment.
Also probably should copy the array so you can't mutate it. (Or maybe this should be done in the constructor? or both?)
There was a problem hiding this comment.
Cool, I think I just copied this from the python impl, but that makes sense!
| throw Error("expected multirange subtype to be scalar type"); | ||
| } | ||
| ctx.imports.add("MultiRange"); | ||
| return `MultiRange<${subCodec.tsType}>`; |
There was a problem hiding this comment.
You can have ranges of LocalDate/Time, so we also need to check if the subCodec type needs adding to ctx.imports
There was a problem hiding this comment.
This mirrors what we're doing in Range, do you think that's also broken? Looking at how ArrayCodec is done here, maybe I need to wrap this and the Range subcodec in a recursive call to walkCodec?
There was a problem hiding this comment.
Yeah, I think you're right, Range also looks like it has the same problem. Either walkCodec or since we know the subCodec is going to be a scalar we could just inline the codec.importedType check, both ways seem fine to me.
There was a problem hiding this comment.
I'll stick with walkCodec just in case other behavior falls out and that seems to match how we're doing other types with subcodecs. Thanks for catching this!
|
While we're messing with ranges, could we also add this small fix to the range constructor: https://github.com/edgedb/edgedb-js/pull/711/files#diff-96a05a9179de11464b9faa257493af517dcd41f1d1b188b059a748dbd55e463aR29 |
Also defensively copy both on the way in and the way out
It should include the lower bound if `_lower` is not `null` or `undefined`
scotttrinh
force-pushed
the
fix-multirange-anytype
branch
from
52369ee to
c0c7210
Compare
A few new features have been developed in 4.0 that needed some updates here to get the driver and query builder to work:
multirangewhich is basically a collection type of ranges.anyobjectwhich is a subset ofanytype.There was also a change to cardinality inference that made certain polymorphic queries an error condition now, and that has been addressed here as well.